New collumn of execution time added in db schema of oc_jobs #27144

noveens · 2017-02-12T23:54:35Z

Description

I have changed the schema of the db_structure.xml file and the corresponding tests where oc_jobs was concerned.

I added a column of execution duration in the oc_jobs table.

Related Issue

#27137

Motivation and Context

This is a new feature which could help in testing of the system.

How Has This Been Tested?

I have tested this thoroughly, even edited one of the tests because of the database migration.

Screenshots (if appropriate):

Not necessary.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2017-02-12T23:54:36Z

@noveens, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bantu, @tanghus and @butonic to be potential reviewers.

CLAassistant · 2017-02-12T23:55:36Z

All committers have signed the CLA.

PVince81 · 2017-02-13T08:36:38Z

This will likely not be enough.

In past ownCloud versions, the XML file was the authority regarding database schemas.
Since master / 10.0, we should use migrations to make database changes.

Have a look how it's done here: https://github.com/owncloud/core/blob/master/core/Migrations/Version20170111103310.php#L9.
Some docs about migrations: owncloud-archive/documentation#2742 (unfinished as this is brand new).

Let me know if you need assistance.

Once this is done the next step is to adjust the background job runner to measure the execution time and put it there.

noveens · 2017-02-13T08:48:57Z

@PVince81 , thanks for the quick review, I'll look into it as soon as possible.
Thanks for the links to ducumentation as well.

noveens · 2017-02-13T21:16:54Z

@PVince81 , please check now, I completed making another column in oc_jobs table named execution time through migration.

DeepDiver1975 · 2017-02-13T21:35:48Z

tests/lib/DB/schemDiffData/core.xml

@@ -732,6 +732,14 @@
 				<notnull>false</notnull>
 			</field>

+			<field>


DeepDiver1975 · 2017-02-13T21:35:54Z

tests/lib/DB/schemDiffData/unsigned.xml

@@ -61,6 +61,14 @@
 				<notnull>false</notnull>
 			</field>

+			<field>


@DeepDiver1975 ,
So I shouldn't add the new column of execution_duration in oc_jobs table's schema of the tests?

no this is not necessary

removing it.

DeepDiver1975 · 2017-02-13T22:17:27Z

@noveens THx - next would be to fill the columns 😉

noveens · 2017-02-13T23:23:28Z

@DeepDiver1975 , does that come in the scope of this bug?

PVince81 · 2017-02-14T08:42:33Z

@noveens yes, because else the column is completely useless.

Do you need pointers ?

noveens · 2017-02-14T08:54:12Z

@PVince81, pointers would be a great way to start.

PVince81 · 2017-02-14T09:13:22Z

The job is executed here: https://github.com/owncloud/core/blob/master/cron.php#L122

I suggest you add statements around here https://github.com/owncloud/core/blob/master/lib/private/BackgroundJob/Job.php#L52 to measure the running time. Then store the value in the database.

For storing, use something like https://github.com/owncloud/core/blob/master/lib/private/BackgroundJob/JobList.php#L307. Maybe a new method setLastRunTime().

noveens · 2017-02-14T12:39:43Z

@PVince81 , I have made the new method, but how to check the changes I have made?

Update: Pushed the changes I made, please review @DeepDiver1975

PVince81 · 2017-02-14T12:57:55Z

lib/private/BackgroundJob/JobList.php

+	public function setExecutionTime($job, $timeTaken) {
+		$query = $this->connection->getQueryBuilder();
+		$query->update('jobs')
+			->set('last_run', $query->createNamedParameter($timeTaken, IQueryBuilder::PARAM_INT))


wrong column ?

sorry, bad mistake :p

@PVince81 , updated

PVince81 · 2017-02-14T13:01:16Z

@noveens please have a look at my comment.

To test this you can run php cron.php manually.
If no job is running because of the schedule, you can reset the jobs manually with the following SQL query:

update oc_jobs set last_run=0,last_checked=0,reserved_at=0;

Then run cron.php again and see if your new column is populated.

noveens · 2017-02-14T14:45:52Z

@PVince81 , please recheck

PVince81 · 2017-02-14T16:08:29Z

Code looks good so far. Thanks.

Will test it later.

PVince81 · 2017-02-14T20:00:39Z

I tested it but after running cron.php I see the last_run being populated, but the value of "execution_duration" is always 0.

Even if my computer is too fast, I'd still expect values of at least a few millisecond.
Is that timestamp in seconds ?

Wanted to test it with LDAP (which provides ways to have slower jobs) but the app is broken at the moment: #27154

noveens · 2017-02-14T21:06:37Z

@PVince81 ,
The new column I have added is of integer type since the time() function gives seconds since epoch
But the general case of checking working time of a job has time lesser than 1 second.

Hence I chose this approach.
If you need any updation, please tell me.

PVince81 · 2017-02-15T08:54:04Z

increase last digit in version.php to trigger an update

PVince81 · 2017-02-15T08:56:39Z

Tested with LDAP. I saw cron.php taking a minute or so, still the column "execution_duration" contains only zeroes.

How did you test it?

noveens · 2017-02-15T20:15:15Z

@PVince81 ,
To be frank, I didn't test it because I couldn't find a way to populate the database.
Also, once populated how should I check the values in the database?

It would be great if you could tell me how to test this.

PVince81 · 2017-02-15T20:17:23Z

Told you here: #27144 (comment)

The table is already pre-populated by default recurring jobs after setting up OC.

noveens · 2017-02-17T22:39:06Z

@bantu, @tanghus, @butonic, @DeepDiver1975 and @PVince81 ,

I managed to pass Jenkins now ( took 1h 42m :p ) please have a look and review this PR.

jvillafanez · 2017-02-20T08:04:04Z

Revert the permission change in the config.sample.php file please.

davitol · 2017-02-20T12:19:28Z

I had to manually increase the OC version to force the trigger of the migration code (checked against daily master).

I tested the PR in the same way.

New column "execution_time" in the "jobs" table with a default value of -1
Running cron will:
set the execution time in seconds of the job in the "jobs" table. 0 is a valid value. It will help to identify what jobs have run and what jobs not. ✅ 
add new logs entries in the owncloud.log. The logs entries should be clear enough to know what jobs have run, and should match the information contained in the "jobs" table.
✅
Need to test against a "heavy" system to verify we're counting the seconds right and not just setting zeros. Probably testing with LDAP active is enough to have some slow jobs which should take more than 1 second to execute. ✅
Find a job with arguments and check the log output. ✅

👍

IMHO A small enhancement when running a job without arguments will be to not print the with arguments: text

{"reqId":"4vkT2Z8Fy\/yzBApDPlqT","remoteAddr":"","app":"cron","message":"Finished background job, the job took : 0 seconds, this job is an instance of class : OCA\\Files\\BackgroundJob\\CleanupFileLocks with arguments : ","level":0,"time":"2017-02-20T12:08:15+00:00","method":"--","url":"--","user":"--"}

noveens · 2017-02-20T13:59:49Z

@jvillafanez ,
I didn't get the part I have to change,
you're talking about which permissions?

jvillafanez · 2017-02-21T08:08:13Z

That change in the permissions of the config.sample.php . It shouldn't appear.

noveens · 2017-02-21T10:54:20Z

@jvillafanez ,
pushed the changes.

noveens · 2017-02-21T10:58:44Z

@DeepDiver1975 ,
please re-review this

jvillafanez · 2017-02-21T11:39:01Z

I don't have anything more to add 👍

noveens · 2017-02-23T10:11:47Z

@PVince81 ,
Can this be merged, I think it's fine now.

PVince81 · 2017-02-23T10:12:47Z

@DeepDiver1975 can you kick Jenkins to retest this ?

noveens · 2017-02-23T12:30:33Z

@PVince81 ,
This passed jenkins before my previous commit, which was just modifying the file permissions of config.sample.php which i suppose has nothing to do with any of the tests.

PVince81 · 2017-02-23T12:32:37Z

Added to new features page: https://github.com/owncloud/core/wiki/ownCloud-10.0-Features

jvillafanez · 2017-02-23T14:02:44Z

@PVince81 @DeepDiver1975 Backport the logging part? I think it could be quite useful and not intrusive. DB changes won't be backported.

PVince81 · 2017-02-23T14:10:57Z

Yes, please backport the logging part.

@noveens can you do it ? Basically create a branch off "stable9.1", put your commits there then submit a PR that targets stable9.1. Only put the logging part there.

noveens · 2017-02-23T14:20:30Z

@PVince81 ,
Sure I can do that.
Just the logging part right?
And do I have to update anything in this branch?

PVince81 · 2017-02-23T14:38:18Z

Nothing to update here. Make a new branch.

[Stable9.1] Backporting logging changes of PR #27144

PVince81 · 2017-02-27T08:10:45Z

stable9.1 logging changes backport: #27239

lock · 2019-08-03T21:02:11Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

New collumn of execution time added in db schema of oc_jobs

050f031

PVince81 mentioned this pull request Feb 13, 2017

Add column for execution duration in oc_jobs table #27137

Closed

noveens added 2 commits February 14, 2017 02:37

Finally completed adding a column to oc_jobs through migration.

4a4fbb5

trailing space.

c86fa8a

DeepDiver1975 requested changes Feb 13, 2017

View reviewed changes

Tests updation not required.

fab44e2

New method added to store execution duration in oc_jobs

108444d

PVince81 reviewed Feb 14, 2017

View reviewed changes

wrong column fixed

7ca8316

called setExecutionTime at wrong place, fixed now.

6929f81

noveens added 4 commits February 17, 2017 23:54

defination of setExecutionTime() added in public.

008f78a

trying to pass jenkins

c43ed8c

trying to pass jenkins

b394ec6

trying to pass jenkins

8b6dc6d

PVince81 added the 3 - To Review label Feb 21, 2017

PVince81 added this to the 10.0 milestone Feb 21, 2017

config.sample.php permissions changed

7cd78c6

PVince81 merged commit ea59b53 into owncloud:master Feb 23, 2017

noveens added a commit to noveens/core that referenced this pull request Feb 23, 2017

backporting logging changes of PR owncloud#27144

75df4ea

noveens mentioned this pull request Feb 23, 2017

[Stable9.1] Backporting logging changes of PR #27144 #27239

Merged

PVince81 pushed a commit that referenced this pull request Feb 27, 2017

Merge pull request #27239 from noveens/backport-oc_jobs

d61600c

[Stable9.1] Backporting logging changes of PR #27144

lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019

New collumn of execution time added in db schema of oc_jobs #27144

New collumn of execution time added in db schema of oc_jobs #27144

Conversation

noveens commented Feb 12, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

mention-bot commented Feb 12, 2017

CLAassistant commented Feb 12, 2017 • edited Loading

PVince81 commented Feb 13, 2017

noveens commented Feb 13, 2017

noveens commented Feb 13, 2017

DeepDiver1975 Feb 13, 2017

Choose a reason for hiding this comment

DeepDiver1975 Feb 13, 2017

Choose a reason for hiding this comment

noveens Feb 13, 2017 • edited Loading

Choose a reason for hiding this comment

DeepDiver1975 Feb 13, 2017

Choose a reason for hiding this comment

noveens Feb 13, 2017

Choose a reason for hiding this comment

DeepDiver1975 commented Feb 13, 2017

noveens commented Feb 13, 2017

PVince81 commented Feb 14, 2017

noveens commented Feb 14, 2017

PVince81 commented Feb 14, 2017

noveens commented Feb 14, 2017 • edited Loading

PVince81 Feb 14, 2017

Choose a reason for hiding this comment

noveens Feb 14, 2017

Choose a reason for hiding this comment

noveens Feb 14, 2017

Choose a reason for hiding this comment

PVince81 commented Feb 14, 2017

noveens commented Feb 14, 2017

PVince81 commented Feb 14, 2017

PVince81 commented Feb 14, 2017

noveens commented Feb 14, 2017 • edited Loading

PVince81 commented Feb 15, 2017

PVince81 commented Feb 15, 2017

noveens commented Feb 15, 2017

PVince81 commented Feb 15, 2017

noveens commented Feb 17, 2017

jvillafanez commented Feb 20, 2017

davitol commented Feb 20, 2017 • edited Loading

noveens commented Feb 20, 2017

jvillafanez commented Feb 21, 2017

noveens commented Feb 21, 2017

noveens commented Feb 21, 2017

jvillafanez commented Feb 21, 2017

noveens commented Feb 23, 2017

PVince81 commented Feb 23, 2017

noveens commented Feb 23, 2017

PVince81 commented Feb 23, 2017

jvillafanez commented Feb 23, 2017

PVince81 commented Feb 23, 2017

noveens commented Feb 23, 2017

PVince81 commented Feb 23, 2017

PVince81 commented Feb 27, 2017

lock bot commented Aug 3, 2019

noveens commented Feb 12, 2017 •

edited

Loading

CLAassistant commented Feb 12, 2017 •

edited

Loading

noveens Feb 13, 2017 •

edited

Loading

noveens commented Feb 14, 2017 •

edited

Loading

noveens commented Feb 14, 2017 •

edited

Loading

davitol commented Feb 20, 2017 •

edited

Loading